Revamp the integration tests #51
Conversation
Refactor the integration test to: 1. Uninstall the CVO and any existing Cluster Ingress Operator deployment 2. Re-install the operator 3. Do a smoke test against the default router 5. Tear everything down This is less than ideal long term and should probably be replaced by proper e2e tests, but in the meantime it should provide a more uniform testing strategy as we iterate.
openshift-ci-robot
added
size/L
|
Here's what success looks like. First, pushing a new operator image from my local tree: Then, running the test using the newly pushed image: |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
test/integration/integration_test.go
Outdated
| sdk.Watch(resource, kind, tc.operatorNamespace, resyncPeriod) | ||
| sdk.Handle(stub.NewHandler()) | ||
| go sdk.Run(context.TODO()) | ||
| func (tc *TestConfig) uninstallOperator(t *testing.T) { |
There was a problem hiding this comment.
Should all this teardown stuff go in a script so it can be called outside the code? That's what I do for manual testing anyway
hack/release-local.sh
Outdated
| @@ -0,0 +1,32 @@ | |||
| #/bin/bash | |||
test/integration/integration_test.go
Outdated
| t.Fatalf("KUBECONFIG is required") | ||
| } | ||
| // The operator-sdk uses KUBERNETES_CONFIG... | ||
| os.Setenv("KUBERNETES_CONFIG", kubeConfig) |
There was a problem hiding this comment.
Is this os.Setenv still needed now that test-integration.sh sets KUBERNETES_CONFIG?
| echo "Pushed $REPO:$REV" | ||
| echo "Install manifests using:" | ||
| echo "" | ||
| echo "oc apply -f $MANIFESTS" |
test/integration/integration_test.go
Outdated
|
|
||
| return config | ||
| // reinstall the operator | ||
| tc.runShellCmdNonFatal(t, fmt.Sprintf(`oc apply -f %s`, tc.manifestsDir)) |
|
I want to try a commit that extracts a lot of this into steps that can be composed |
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "router-default", | ||
| Namespace: "openshift-cluster-ingress-router", |
There was a problem hiding this comment.
Shouldn't the namespace be namespace?
There was a problem hiding this comment.
Clarified this in a comment
There was a problem hiding this comment.
Is that the // Hard-coded reference to the default router. comment? I guess I didn't realize the test was using a router deployed outside the test. Seems a little strange that the operator's "TestDefaultIngress" integration test doesn't use the operator to set up a default ingress—is the plan eventually to do that?
test/integration/integration_test.go
Outdated
| func NewTestConfig(t *testing.T) *TestConfig { | ||
| config := &TestConfig{t: t} | ||
|
|
||
| func NewTestConfig(t *testing.T, clusterName string, manifestsDir string) *TestConfig { |
There was a problem hiding this comment.
It's a little confusing to use the same names for the parameters as you used for the global variables for the flags.
test/integration/integration_test.go
Outdated
| tc.runShellCmdNonFatal(t, `oc delete clusterroles/cluster-ingress:router`) | ||
| tc.runShellCmdNonFatal(t, `oc delete clusterrolebindings/cluster-ingress-operator:operator`) | ||
| tc.runShellCmdNonFatal(t, `oc delete clusterrolebindings/cluster-ingress:router`) | ||
| tc.runShellCmdNonFatal(t, `oc delete customresourcedefinition.apiextensions.k8s.io/clusteringresses.ingress.openshift.io`) |
There was a problem hiding this comment.
Could you replace all the individual deletes with tc.runShellCmdNonFatal(t, fmt.Sprintf("oc delete -f %s -R", tc.manifestsDir))?
There was a problem hiding this comment.
Extracted all this into a script
There was a problem hiding this comment.
Not sure about using -R with oc delete in this case; I intentionally ordered the delete to minimize contention (e.g. tearing down the operator before routers to avoid fighting with the operator which wants to keep creating routers). Although with --force --grace-period 0 on the namespaces themselves I'm not entirely sure it matters. Worth doing some experiments.
There was a problem hiding this comment.
If it's not a big deal I'd ask we keep what I have in the script for now (which works) and optimize it later if possible.
Separate the component parts of the integration tests to faciliate manual testing and to simplify the testing code.
openshift-ci-robot
added
size/XL
|
Okay, I redid a lot of this in an attempt to make the whole thing simpler and also more flexible. With a few additional steps on the part of the user, it should be possible to make use of all this stuff for both verifying our work outside CI and also iterative development through ad-hoc use of each piece (image builds, test, uninstaller). PTAL! |
| metadata: | ||
| name: cluster-ingress-test | ||
| labels: | ||
| openshift.io/cluster-ingress-operator-test: "" |
There was a problem hiding this comment.
Reserved for future use: should make it easier to identify test namespaces for cleanup.
hack/test-integration.sh
Outdated
|
|
||
| if [ -z "${CLUSTER_NAME}" ]; then echo "CLUSTER_NAME is required"; exit 1; fi | ||
| if [ -z "${MANIFESTS}" ]; then echo "MANIFESTS is required"; exit 1; fi | ||
| if [ -z "${KUBECONFIG}" ]; then echo "KUBECONFIG is required"; exit 1; fi |
There was a problem hiding this comment.
Should use ${KUBECONFIG:-} here or KUBECONFIG="${KUBECONFIG:-} above or else you will get "bash: KUBECONFIG: unbound variable" instead of the desired error message.
hack/uninstall.sh
Outdated
| oc delete clusterroles/cluster-ingress:router || true | ||
| oc delete clusterrolebindings/cluster-ingress-operator:operator || true | ||
| oc delete clusterrolebindings/cluster-ingress:router || true | ||
| oc delete customresourcedefinition.apiextensions.k8s.io/clusteringresses.ingress.openshift.io || true |
There was a problem hiding this comment.
Instead of ending every command with || true, you could just omit the set -e.
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "router-default", | ||
| Namespace: "openshift-cluster-ingress-router", |
There was a problem hiding this comment.
Is that the // Hard-coded reference to the default router. comment? I guess I didn't realize the test was using a router deployed outside the test. Seems a little strange that the operator's "TestDefaultIngress" integration test doesn't use the operator to set up a default ingress—is the plan eventually to do that?
hack/uninstall.sh
Outdated
| # Uninstall tectonic ingress | ||
| oc delete namespaces/openshift-ingress || true | ||
|
|
||
| # Uninstall cluster-dns-operator |
Yes, the test executes against a cluster where the operator is assumed to already be running. For now at least, this is the only way we can consistently ensure our changes actually work inside a 4.0 cluster, including exercising RBAC, admission control within an operator namespace, etc. This is probably technically now more of an e2e test, but some sort of consistent tooling for iterating like this was desperately needed. If anything, I'd imagine the tooling will continue to be of greater use than the test itself as the e2e tests are developed (e.g. you could use the uninstall/release image tooling to set up a cluster for iterating on e2e tests that live in origin and which execute with CI). I don't see a lot of useful middle ground between unit and e2e tests at the moment- we had too many problems which only presented themselves only when running in-cluster. |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ironcladlou, Miciah The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| echo "Pushed $REPO:$REV" | ||
| echo "Install manifests using:" | ||
| echo "" | ||
| echo "oc apply -f $MANIFESTS" |
6 participants
Refactor the integration test to:
This is less than ideal long term and should probably be replaced by proper e2e
tests, but in the meantime it should provide a more uniform testing strategy as
we iterate.